-
Notifications
You must be signed in to change notification settings - Fork 13.6k
Port #[link]
to the new attribute parsing infrastructure
#143193
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: master
Are you sure you want to change the base?
Conversation
47c45bb
to
aaf7d53
Compare
This comment has been minimized.
This comment has been minimized.
☔ The latest upstream changes (presumably #143233) made this pull request unmergeable. Please resolve the merge conflicts. |
aaf7d53
to
d43b4b5
Compare
This comment has been minimized.
This comment has been minimized.
147aeec
to
29b3648
Compare
29b3648
to
6b10a20
Compare
PrintAttribute | ||
)] | ||
#[derive(HashStable_Generic)] | ||
pub enum NativeLibKind { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This type was moved here because it is needed for the link attribute and the original location caused a dependency cycle
|
||
#[link(name = "bar", import_name_type = "decorated", kind = "raw-dylib")] | ||
#[link(name = "bar", kind = "raw-dylib")] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This test is not testing for the invalid attribute, but instead for the link attribute on an extern type. Because attributes are parsed earlier and this attribute was invalid the error blocked the crash from happening. This is true for quite a few other of the test changes as well
@@ -81,8 +81,7 @@ | |||
#[export_stable = 1] | |||
//~^ ERROR malformed | |||
#[link] | |||
//~^ ERROR attribute must be of the form | |||
//~| WARN this was previously accepted by the compiler | |||
//~^ ERROR malformed |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
BREAKING CHANGE 1: A #[link]
attribute which is not a list #[link(...)]
used to be a warning future error, is now an error. This needs a crater run.
@@ -61,7 +61,7 @@ | |||
#![doc = "2400"] | |||
#![cold] //~ WARN attribute should be applied to a function | |||
//~^ WARN this was previously accepted | |||
#![link()] //~ WARN attribute should be applied to an `extern` block | |||
#![link(name = "x")] //~ WARN attribute should be applied to an `extern` block |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
BREAKING CHANGE 2: A link attribute applied to an invalid target was not checked for syntactic correctness, it now is. The invalid target remains a warning, but the syntactic corectness is now checked
@rustbot ready Needs a crater run |
|
Some changes occurred in compiler/rustc_passes/src/check_attr.rs Some changes occurred in compiler/rustc_attr_parsing Some changes occurred in compiler/rustc_codegen_ssa These commits modify the If this was unintentional then you should revert the changes before this PR is merged. Some changes occurred in compiler/rustc_attr_data_structures This PR changes a file inside |
@bors try @craterbot queue |
Port `#[link]` to the new attribute parsing infrastructure Ports `link` to the new attribute parsing infrastructure for #131229 (comment) This rework is not yet finished, it is blocked on the `cfg` attribute being finished, since `link` takes a `cfg` attribute as argument `@rustbot` blocked
☀️ Try build successful - checks-actions |
@craterbot queue |
🚨 Error: failed to parse the command 🆘 If you have any trouble with Crater please ask in t-infra on Zulip |
@craterbot check oops |
🚨 Error: failed to parse the command 🆘 If you have any trouble with Crater please ask in t-infra on Zulip |
@craterbot check |
👌 Experiment ℹ️ Crater is a tool to run experiments across parts of the Rust ecosystem. Learn more |
🚧 Experiment ℹ️ Crater is a tool to run experiments across parts of the Rust ecosystem. Learn more |
🎉 Experiment
|
These are a bunch of real regressions sadly. I don't think we should break these popular crates, even though they are very old versions. Pinging @rust-lang/lang to decide |
Thanks, we'll discuss. Looking at the crater results, it's worth noting that almost all of the breakage is on GH repositories; very few crates on |
I wonder, could we ask the winit and glutin folks to upload a new micro version of those past major versions, with the only change being to fix the link syntax? |
Sounds like a reasonable solution. |
@JonathanBrouwer Given the age of those crates, I don't think any addition of parsing logic here would be temporary. How much pain would that parsing logic be to handle the |
@joshtriplett It should be easy, just an extra if statement at the start of the parser where it currently only accepts a list, just a few lines of code. What should the attribute actually do? Afaik before this PR the attribute is completely ignored, so I propose to keep it that way. I'll double check that this is what happens |
We talked about this in lang triage. Here's what I'd propose we do. Let's:
@rfcbot fcp merge |
Team member @traviscross has proposed to merge this. The next step is review by the rest of the tagged team members: No concerns currently listed. Once a majority of reviewers approve (and at most 2 approvals are outstanding), this will enter its final comment period. If you spot a major issue that hasn't been raised at any point in this process, please speak up! cc @rust-lang/lang-advisors: FCP proposed for lang, please feel free to register concerns. |
Ports
link
to the new attribute parsing infrastructure for #131229 (comment)